Conversation
Adds a new disk usage component that displays storage information for mounted filesystems. The component includes a progress bar visualization and shows used/total space per disk. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a disk-usage feature: a server endpoint that returns parsed Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (React DiskUsage)
participant Server as Server (API)
participant System as Host OS
Client->>Client: mount DiskUsage component
loop every refreshRate
Client->>Server: GET /api/disks
Server->>System: execute `df -h`
System-->>Server: raw df output
Server->>Server: parse/filter into DiskInfo[]
Server-->>Client: 200 { disks: DiskInfo[] }
Client->>Client: update state, compute totals, render bars
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/components/DiskUsage.tsx:
- Around line 22-43: Replace the hardcoded fetch URL in fetchDiskInfo with a
configurable one (use a relative path like '/api/disks' or derive from an env
var) so it works in production, change console.log to console.error when logging
errors, and add an AbortController inside the effect: pass controller.signal to
fetch, catch and ignore AbortError (or skip setDisks/setLoading/setError when
err.name === 'AbortError'), and call controller.abort() in the cleanup along
with clearInterval; keep using the existing fetchDiskInfo, setDisks, setLoading,
setError, and refreshRate identifiers so changes are localized.
🧹 Nitpick comments (9)
server/index.ts (3)
244-248: Redundant platform-specific branching.Both branches execute the identical command
"df -h". Either remove the conditional or implement platform-specific differences if intended.Suggested simplification
async function getDiskInfo(): Promise<DiskInfo[]> { - const os = platform(); - let cmd: string; - - if (os === "darwin") { - cmd = "df -h"; - } else { - cmd = "df -h"; - } + const cmd = "df -h";
263-265: Consider explicit radix and broader device filtering.
parseInt(percentStr)should use explicit radix:parseInt(percentStr, 10).- The filter
filesystem.startsWith("/dev") || filesystem.includes("disk")may exclude valid block devices on some systems (e.g., LVM devices under/dev/mapper/should already match, but network mounts like NFS won't). Verify this matches the intended filtering behavior.Add explicit radix
- const percent = parseInt(percentStr); + const percent = parseInt(percentStr, 10);
279-279: Consider removing or reducing verbosity of info log.This
console.logexecutes on every request (potentially every second based on refresh rate), which could be noisy. Consider removing it or guarding it behind a debug/verbose flag.src/App.tsx (1)
63-63: Consider consolidating data fetching pattern.The
DiskUsagecomponent fetches its own data independently, whileCpuGraphandMemoryGraphreceive data fromuseSystemMetrics. This creates two separate polling intervals to the backend. Consider adding disk info to the/api/metricsendpoint anduseSystemMetricshook for a unified data flow and single network request.That said, the current approach is functional and keeps disk fetching isolated, which may be intentional if disk operations are slower or should fail independently.
src/components/DiskUsage.tsx (5)
45-56: Consider using CSS variables for consistency.The color values are hardcoded but match the CSS variables in
App.css(--color-green,--color-yellow,--color-orange,--color-red). Using CSS custom properties viavar(--color-green)etc. would ensure consistency if the theme changes.
58-73: Consider memoizing totals calculation.
calculateTotals()is called on every render. Since it depends only ondisks, wrapping it withuseMemowould prevent unnecessary recalculations.Suggested optimization
+ import { useState, useEffect, useMemo } from 'react'; ... - const calculateTotals = () => { - let totalUsed = 0; - let totalSize = 0; - - for (let i = 0; i < disks.length; i++) { - const disk = disks[i]; - const sizeNum = parseSize(disk.size); - const usedNum = parseSize(disk.used); - totalSize = totalSize + sizeNum; - totalUsed = totalUsed + usedNum; - } - - return { totalUsed, totalSize }; - }; ... - const totals = calculateTotals(); + const totals = useMemo(() => { + let totalUsed = 0; + let totalSize = 0; + for (const disk of disks) { + totalSize += parseSize(disk.size); + totalUsed += parseSize(disk.used); + } + return { totalUsed, totalSize }; + }, [disks]);
75-93: TODO acknowledged - utility function works correctly.The
parseSizefunction handles commondf -houtput formats. The TODO comment indicates intent to extract this to a utility file, which would enable testing and reuse.Would you like me to create an issue to track moving this to a shared utility file?
95-101: Use strict equality.Line 96 uses loose equality (
==). Prefer strict equality (===) for consistency and to avoid type coercion surprises.Fix
- if (bytes == 0) return '0 B'; + if (bytes === 0) return '0 B';
124-144: Consider using a stable key instead of index.Using
indexas the React key (line 126) works when the list is stable, but if disks can change order or be added/removed mid-list, it may cause rendering issues. Consider usingdisk.mountedordisk.filesystemas the key for better stability.Suggested change
- {displayDisks.map((disk, index) => ( - <div key={index} className="disk-item"> + {displayDisks.map((disk) => ( + <div key={disk.mounted} className="disk-item">
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/index.tssrc/App.csssrc/App.tsxsrc/components/DiskUsage.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/App.tsx (1)
src/components/DiskUsage.tsx (1)
DiskUsage(16-157)
src/components/DiskUsage.tsx (1)
server/index.ts (1)
fetch(313-392)
🔇 Additional comments (6)
server/index.ts (1)
346-354: LGTM!The endpoint follows the established patterns in this file and integrates well with the existing API structure.
src/App.css (2)
707-716: LGTM!The styling follows the existing design patterns and CSS variable usage. The
grid-column: span 2makes sense for a full-width panel in the metrics grid.
718-813: Styling looks good.The disk item, bar, and expand button styles are consistent with the existing design language. Transitions and hover effects follow established patterns.
src/App.tsx (1)
8-8: LGTM!Import is correctly placed with other component imports.
src/components/DiskUsage.tsx (2)
3-14: LGTM!The
DiskInfointerface correctly mirrors the server-side type, and the props interface is appropriately minimal.
147-154: LGTM!The expand/collapse functionality is clean and the conditional rendering is appropriate.
| useEffect(() => { | ||
| const fetchDiskInfo = async () => { | ||
| try { | ||
| const response = await fetch('http://localhost:3001/api/disks'); | ||
| if (!response.ok) { | ||
| throw new Error('Failed to fetch disk info'); | ||
| } | ||
| const data = await response.json(); | ||
| setDisks(data.disks); | ||
| setLoading(false); | ||
| setError(null); | ||
| } catch (err) { | ||
| console.log('Error fetching disk info:', err); | ||
| setError('Failed to load disk info'); | ||
| setLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetchDiskInfo(); | ||
| const interval = setInterval(fetchDiskInfo, refreshRate); | ||
| return () => clearInterval(interval); | ||
| }, [refreshRate]); |
There was a problem hiding this comment.
Hardcoded URL and missing fetch abort.
- The hardcoded
http://localhost:3001URL will break in production. Consider using an environment variable or relative URL with a proxy configuration. - Use
console.errorinstead ofconsole.logfor error logging (line 34). - Consider adding an
AbortControllerto cancel in-flight requests when the component unmounts orrefreshRatechanges, preventing state updates on unmounted components.
Suggested improvements
useEffect(() => {
+ const controller = new AbortController();
+
const fetchDiskInfo = async () => {
try {
- const response = await fetch('http://localhost:3001/api/disks');
+ const response = await fetch('/api/disks', { signal: controller.signal });
if (!response.ok) {
throw new Error('Failed to fetch disk info');
}
const data = await response.json();
setDisks(data.disks);
setLoading(false);
setError(null);
} catch (err) {
- console.log('Error fetching disk info:', err);
- setError('Failed to load disk info');
- setLoading(false);
+ if (err instanceof Error && err.name !== 'AbortError') {
+ console.error('Error fetching disk info:', err);
+ setError('Failed to load disk info');
+ setLoading(false);
+ }
}
};
fetchDiskInfo();
const interval = setInterval(fetchDiskInfo, refreshRate);
- return () => clearInterval(interval);
+ return () => {
+ controller.abort();
+ clearInterval(interval);
+ };
}, [refreshRate]);🤖 Prompt for AI Agents
In @src/components/DiskUsage.tsx around lines 22 - 43, Replace the hardcoded
fetch URL in fetchDiskInfo with a configurable one (use a relative path like
'/api/disks' or derive from an env var) so it works in production, change
console.log to console.error when logging errors, and add an AbortController
inside the effect: pass controller.signal to fetch, catch and ignore AbortError
(or skip setDisks/setLoading/setError when err.name === 'AbortError'), and call
controller.abort() in the cleanup along with clearInterval; keep using the
existing fetchDiskInfo, setDisks, setLoading, setError, and refreshRate
identifiers so changes are localized.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/index.ts`:
- Around line 262-275: The current conditional that filters mounted filesystems
(the if guarding the diskList.push block that checks
filesystem.startsWith("/dev") || filesystem.includes("disk")) is excluding valid
mounts; remove that filter so every parsed mount entry is pushed into diskList
(or replace it with an explicit allow/deny based on filesystem type if you
prefer). Locate the block that builds diskList using variables filesystem,
parts, percentStr/percent, and mounted, and either delete the surrounding if
(...) { ... } or change it to check an explicit whitelist/blacklist of
filesystem types before calling diskList.push so all intended mounts are
returned.
- Around line 257-274: The current parsing splits df output on whitespace and
uses parts[5] for the mount path, which truncates mountpoints containing spaces;
update the parsing in the block that builds diskList (variables: parts,
filesystem, percentStr, percent, mounted) to construct the mount path from the
tail of the parts array (e.g., mounted = parts.slice(5).join(' ')), keep percent
parsing from parts[4] as-is but validate parseInt result, and then push the disk
object with the joined mounted path so full mount paths like "/Volumes/My Drive"
are preserved.
In `@src/App.css`:
- Around line 708-716: The .disk-usage rule uses grid-column: span 2 which can
create an implicit column and break layouts when .metrics-panel collapses to one
column; update the .disk-usage CSS to use grid-column: 1 / -1 so it spans the
full row across all breakpoints (replace grid-column: span 2 with grid-column: 1
/ -1 in the .disk-usage selector) to avoid implicit columns and ensure correct
responsive behavior.
🧹 Nitpick comments (1)
src/components/DiskUsage.tsx (1)
75-93: Consider extractingparseSizeas noted in the TODO.If you want, I can help factor this into a shared utility (and add tests) to keep the component lean.
| const line = lines[i]; | ||
| const parts = line.trim().split(/\s+/); | ||
|
|
||
| if (parts.length >= 6) { | ||
| const filesystem = parts[0]; | ||
| // filter out virtual filesystems | ||
| if (filesystem.startsWith("/dev") || filesystem.includes("disk")) { | ||
| const percentStr = parts[4].replace("%", ""); | ||
| const percent = parseInt(percentStr); | ||
|
|
||
| diskList.push({ | ||
| filesystem: filesystem, | ||
| size: parts[1], | ||
| used: parts[2], | ||
| available: parts[3], | ||
| use_percent: percent, | ||
| mounted: parts[5], | ||
| }); |
There was a problem hiding this comment.
Preserve mount paths that contain spaces.
Splitting on whitespace and using parts[5] truncates mountpoints with spaces (e.g., “/Volumes/My Drive”). Consider joining the tail for the mount path.
🛠️ Suggested parsing fix
- if (parts.length >= 6) {
- const filesystem = parts[0];
+ if (parts.length >= 6) {
+ const filesystem = parts[0];
+ const mounted = parts.slice(5).join(" ");
const percentStr = parts[4].replace("%", "");
const percent = parseInt(percentStr);
diskList.push({
filesystem: filesystem,
size: parts[1],
used: parts[2],
available: parts[3],
use_percent: percent,
- mounted: parts[5],
+ mounted: mounted,
});
}🤖 Prompt for AI Agents
In `@server/index.ts` around lines 257 - 274, The current parsing splits df output
on whitespace and uses parts[5] for the mount path, which truncates mountpoints
containing spaces; update the parsing in the block that builds diskList
(variables: parts, filesystem, percentStr, percent, mounted) to construct the
mount path from the tail of the parts array (e.g., mounted =
parts.slice(5).join(' ')), keep percent parsing from parts[4] as-is but validate
parseInt result, and then push the disk object with the joined mounted path so
full mount paths like "/Volumes/My Drive" are preserved.
| // filter out virtual filesystems | ||
| if (filesystem.startsWith("/dev") || filesystem.includes("disk")) { | ||
| const percentStr = parts[4].replace("%", ""); | ||
| const percent = parseInt(percentStr); | ||
|
|
||
| diskList.push({ | ||
| filesystem: filesystem, | ||
| size: parts[1], | ||
| used: parts[2], | ||
| available: parts[3], | ||
| use_percent: percent, | ||
| mounted: parts[5], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Return all mounted filesystems; current filter drops valid entries.
The PR objective says “all mounted filesystems,” but filtering to /dev or names containing "disk" will skip mounts like NFS/SMB, tmpfs, overlay, etc. Consider removing the filter (or replacing with a clear allow/deny list based on filesystem type) so the panel reflects all mounts.
💡 Suggested adjustment
- // filter out virtual filesystems
- if (filesystem.startsWith("/dev") || filesystem.includes("disk")) {
- const percentStr = parts[4].replace("%", "");
- const percent = parseInt(percentStr);
-
- diskList.push({
- filesystem: filesystem,
- size: parts[1],
- used: parts[2],
- available: parts[3],
- use_percent: percent,
- mounted: parts[5],
- });
- }
+ const percentStr = parts[4].replace("%", "");
+ const percent = parseInt(percentStr);
+
+ diskList.push({
+ filesystem: filesystem,
+ size: parts[1],
+ used: parts[2],
+ available: parts[3],
+ use_percent: percent,
+ mounted: parts[5],
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // filter out virtual filesystems | |
| if (filesystem.startsWith("/dev") || filesystem.includes("disk")) { | |
| const percentStr = parts[4].replace("%", ""); | |
| const percent = parseInt(percentStr); | |
| diskList.push({ | |
| filesystem: filesystem, | |
| size: parts[1], | |
| used: parts[2], | |
| available: parts[3], | |
| use_percent: percent, | |
| mounted: parts[5], | |
| }); | |
| } | |
| const percentStr = parts[4].replace("%", ""); | |
| const percent = parseInt(percentStr); | |
| diskList.push({ | |
| filesystem: filesystem, | |
| size: parts[1], | |
| used: parts[2], | |
| available: parts[3], | |
| use_percent: percent, | |
| mounted: parts[5], | |
| }); |
🤖 Prompt for AI Agents
In `@server/index.ts` around lines 262 - 275, The current conditional that filters
mounted filesystems (the if guarding the diskList.push block that checks
filesystem.startsWith("/dev") || filesystem.includes("disk")) is excluding valid
mounts; remove that filter so every parsed mount entry is pushed into diskList
(or replace it with an explicit allow/deny based on filesystem type if you
prefer). Locate the block that builds diskList using variables filesystem,
parts, percentStr/percent, and mounted, and either delete the surrounding if
(...) { ... } or change it to check an explicit whitelist/blacklist of
filesystem types before calling diskList.push so all intended mounts are
returned.
| /* Disk Usage Panel */ | ||
| .disk-usage { | ||
| background: var(--bg-card); | ||
| border: 1px solid var(--border-color); | ||
| border-radius: 12px; | ||
| padding: 16px; | ||
| box-shadow: 0 4px 20px rgba(0, 0, 0, 0.3); | ||
| grid-column: span 2; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/App.css | sed -n '700,720p'Repository: coderabbit-demo/btop
Length of output: 561
🏁 Script executed:
# Search for metrics-panel and related grid definitions
rg -A 5 -B 5 "metrics-panel|disk-usage" src/App.css | head -100Repository: coderabbit-demo/btop
Length of output: 725
🏁 Script executed:
# Look for media queries that might affect the grid layout
rg "max-width.*1200px|@media" src/App.css | head -20Repository: coderabbit-demo/btop
Length of output: 120
🏁 Script executed:
# Check the full context and structure around disk-usage
rg -B 15 "\.disk-usage" src/App.css | head -50Repository: coderabbit-demo/btop
Length of output: 450
🏁 Script executed:
# Look for any additional disk-usage rules or media query overrides
rg "\.disk-usage" src/App.cssRepository: coderabbit-demo/btop
Length of output: 97
🏁 Script executed:
# Check the 800px media query context to see if disk-usage needs adjustment there too
rg -A 20 "max-width.*800px" src/App.cssRepository: coderabbit-demo/btop
Length of output: 340
Use grid-column: 1 / -1 for responsive full-width spanning.
When .metrics-panel switches to a single column at max-width 1200px, the current grid-column: span 2 creates an implicit second column, causing unintended layout behavior. Using grid-column: 1 / -1 spans all available columns without creating implicit ones, providing proper responsive behavior across all breakpoints.
🎛️ Suggested fix
.disk-usage {
background: var(--bg-card);
border: 1px solid var(--border-color);
border-radius: 12px;
padding: 16px;
box-shadow: 0 4px 20px rgba(0, 0, 0, 0.3);
- grid-column: span 2;
+ grid-column: 1 / -1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* Disk Usage Panel */ | |
| .disk-usage { | |
| background: var(--bg-card); | |
| border: 1px solid var(--border-color); | |
| border-radius: 12px; | |
| padding: 16px; | |
| box-shadow: 0 4px 20px rgba(0, 0, 0, 0.3); | |
| grid-column: span 2; | |
| } | |
| /* Disk Usage Panel */ | |
| .disk-usage { | |
| background: var(--bg-card); | |
| border: 1px solid var(--border-color); | |
| border-radius: 12px; | |
| padding: 16px; | |
| box-shadow: 0 4px 20px rgba(0, 0, 0, 0.3); | |
| grid-column: 1 / -1; | |
| } |
🤖 Prompt for AI Agents
In `@src/App.css` around lines 708 - 716, The .disk-usage rule uses grid-column:
span 2 which can create an implicit column and break layouts when .metrics-panel
collapses to one column; update the .disk-usage CSS to use grid-column: 1 / -1
so it spans the full row across all breakpoints (replace grid-column: span 2
with grid-column: 1 / -1 in the .disk-usage selector) to avoid implicit columns
and ensure correct responsive behavior.
1 participant
Summary
Test plan
/api/disksendpoint returns disk information🤖 Generated with Claude Code